-
Notifications
You must be signed in to change notification settings - Fork 30
feat: implement r+, r= and r- #114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0a33cb0
to
ea26761
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Left a couple of nits, but otherwise looks very clean.
tester.get_comment().await?, | ||
format!( | ||
"Commit pr-{}-sha has been approved by `{}`", | ||
default_pr_number(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's find to hardcode these values here to make the test simpler. If we ever change them in the future, the test will just fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the reason I'm not hardcoding the values in was I didn't know where those values come from, and this was my attempt to explain the origin of those values for future me 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a bad reason! I hope we could make the default values so obvious that this wouldn't be needed though, and it would be immediately obvious from the test that 1
or default-user
etc. is a special value :)
9169d89
to
42056cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! :)
No description provided.